Repository navigation
Conversation
The bit counter update in PHP_GOSTUpdate() and PHP_SNEFRUUpdate() set the low word one too high when it wrapped, and added at most one carry to the high word per update. Snefru also computed the bit length of a single update in 32 bits. The encoded message length, and thus the digest, was wrong for inputs of 2^29 bytes or more. Compute the counter with 64-bit arithmetic.
ndossche
left a comment
ndossche
left a comment
There was a problem hiding this comment.
This is almost fully the fix I prefer, since 64-bit computing is obiquitous and even on 32-bit systems the more obvious code won't hurt.
I have 2 small code suggestions: if you use a bitwise OR instead of an ADD, then the compiler (at least GCC) generates a 64-bit addition in machine code rather.
I also added a question
| } else { | ||
| context->count[0] += len * 8; | ||
| } | ||
| uint64_t count = ((uint64_t) context->count[1] << 32) + context->count[0] + (uint64_t) len * 8; |
There was a problem hiding this comment.
| uint64_t count = ((uint64_t) context->count[1] << 32) + context->count[0] + (uint64_t) len * 8; | |
| uint64_t count = (((uint64_t) context->count[1] << 32) | context->count[0]) + (uint64_t) len * 8; |
| } else { | ||
| context->count[1] += (uint32_t) len * 8; | ||
| } | ||
| uint64_t count = ((uint64_t) context->count[0] << 32) + context->count[1] + (uint64_t) len * 8; |
There was a problem hiding this comment.
| uint64_t count = ((uint64_t) context->count[0] << 32) + context->count[1] + (uint64_t) len * 8; | |
| uint64_t count = (((uint64_t) context->count[0] << 32) | context->count[1]) + (uint64_t) len * 8; |
| --SKIPIF-- | ||
| <?php | ||
| if (!getenv('RUN_RESOURCE_HEAVY_TESTS')) die('skip resource-heavy test'); | ||
| if (PHP_INT_SIZE != 8) die('skip this test is for 64bit platforms only'); |
There was a problem hiding this comment.
Is that so, why can't this work on 32-bit?
…-bit Use a bitwise OR to combine the two 32-bit words of the bit counter so that compilers emit a single 64-bit addition, and drop the 64-bit-only SKIPIF from the resource-heavy test.
|
Thanks! Applied both suggestions. On 32-bit: there's no reason it can't work; the skip was only out of caution about the single 512 MiB |
PHP_GOSTUpdate()keeps the message length in bits as two 32-bit words and updates it like this:When the low word wraps, the new low word is one too large, because
MAX32is 2^32 - 1 where 2^32 is needed. A single update of 2^30 bytes or more (2^33 bits) should carry more than once, but only one is added to the high word.PHP_SNEFRUUpdate()has the same code and also computes the bit length of the update as(uint32_t) len * 8.The length is part of the final block, so
gost,gost-cryptoandsnefru/snefru256give wrong digests for every input of 2^29 bytes (512 MiB) or more. This also applies tohash_file()and to incrementalhash_update()calls, because the error occurs in whichever update crosses the boundary. Smaller inputs are not affected.Reproducer
Digests of N bytes of
a, compared with RHash 1.4.4 (an independent implementation) built from source:a3464828...a3464828...a3464828...0348bb73...0348bb73...0348bb73...6c30acc1...990c57fe...6c30acc1...0842a8e7...d2727e81...0842a8e7...5e8bdc17...985f7997...5e8bdc17...The 536870912 values before the fix are the same for one
hash()call and for 512hash_update()calls of 1 MiB. After the fix, a single update of 2^30 + 5 bytes gives the same digest as 1024 updates of 1 MiB plus one of 5 bytes, for both algorithms. That case exercises the multiple carry.Fix
Compute the new counter with 64-bit arithmetic and store it back in the two words. The context layout and its serialization are unchanged.
Test
ext/hash/tests/gost_snefru_512mib.phpthashes 2^29 bytes with 512hash_update()calls of 1 MiB forgostandsnefru, plus onehash('snefru', ...)call on a 512 MiB string. The expected digests are from RHash. The test needs about 512 MiB of memory and takes about 40 s, so like the other large-memory tests it only runs withRUN_RESOURCE_HEAVY_TESTSset, on 64-bit platforms, and not withSKIP_SLOW_TESTS. It setsmemory_limit=-1.990c57fe.../d2727e81...for all three lines.RUN_RESOURCE_HEAVY_TESTS, it is skipped.ext/hash/tests: 77 passed, 8 skipped (including the existinggost.phptandsnefru.phptvectors).This issue was found by an LLM-Enhanced Static Analysis Engine and verified manually.
🤖 Generated with Claude Code